Skip to content

parse values with units in FunctionCallDefinition #388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
zeroXbrock merged 7 commits into from
Dec 18, 2025
Merged

parse values with units in FunctionCallDefinition #388

zeroXbrock merged 7 commits into from
Dec 18, 2025

Conversation

@Parizval
Copy link
Contributor

@Parizval Parizval commented Dec 4, 2025 •
edited
Loading

Motivation

Issue: #387

Refactoring data type of Value of FunctionCallDefinition and FunctionCallDefinitionStrict to U256.

PR Checklist

  • Added Tests
  • Added Documentation
  • Ran cargo +nightly clippy --workspace --lib --examples --tests --benches --all-features --locked --fix
  • Ran cargo fmt --all
  • Note breaking changes in PR description, if applicable
  • update changelogs
    • Update CHANGELOG.md in each affected crate
    • add a high-level description in the root changelog

@Parizval Parizval requested a review from zeroXbrock as a code owner December 4, 2025 18:55
zeroXbrock
zeroXbrock requested changes Dec 4, 2025
View reviewed changes
Copy link
Member

@zeroXbrock zeroXbrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, good first pass. I made a mistake in the original spec that requires some changes: FunctionCallDefinition.value actually does need to be Option<String> because the templater relies on strings to support placeholder injection. It's a tiny change on your end, but my mistake -- sorry bout that!

Once the changes I requested are addressed, the code will be ready to support values with units!

@Parizval Parizval requested a review from zeroXbrock December 9, 2025 16:42
@Parizval
Copy link
Contributor Author

Parizval commented Dec 16, 2025

@zeroXbrock, Could you provide your feedback on the changes ?

@zeroXbrock
Copy link
Member

zeroXbrock commented Dec 17, 2025

@zeroXbrock, Could you provide your feedback on the changes ?

yep, reviewing today! 🫡

@zeroXbrock
Copy link
Member

zeroXbrock commented Dec 18, 2025

thanks @Parizval! I realized that I made yet another mistake -- FunctionCallDefinitionStrict is supposed to hold String values -- its only guarantee is that it has a definite from field (where FunctionCallDefinition does not), but otherwise the strings it holds are to be evaluated by the Templater. Sorry for the confusion! Anyhow, I just went ahead and fixed it up. Everything works great -- thanks again for contributing!

@zeroXbrock zeroXbrock changed the title refactor: updated data type of Value of FunctionCallDefinition to U256 parse values with units in FunctionCallDefinition Dec 18, 2025
@zeroXbrock zeroXbrock merged commit 3a7830b into flashbots:main Dec 18, 2025
7 checks passed
@zeroXbrock zeroXbrock mentioned this pull request Dec 18, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zeroXbrock zeroXbrock zeroXbrock approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Parizval @zeroXbrock